Test concurrent feecurrency tx handling in tx pool #2333
Conversation
When handling fee currency transactions.
|
Coverage from tests in coverage: 55.4% of statements across all listed packagescoverage: 68.6% of statements in consensus/istanbul coverage: 63.6% of statements in consensus/istanbul/announce coverage: 57.6% of statements in consensus/istanbul/backend coverage: 0.0% of statements in consensus/istanbul/backend/backendtest coverage: 24.3% of statements in consensus/istanbul/backend/internal/replica coverage: 66.6% of statements in consensus/istanbul/core coverage: 50.0% of statements in consensus/istanbul/db coverage: 0.0% of statements in consensus/istanbul/proxy coverage: 64.2% of statements in consensus/istanbul/uptime coverage: 52.4% of statements in consensus/istanbul/validator coverage: 79.2% of statements in consensus/istanbul/validator/random |
|
|
There was a problem hiding this comment.
LGTM
The only doubt I have, is that you are sending the txs from the same N accounts, and I thought that every txpool had a maximum amount of txs per account in a pending state, so I would have guess that it was going to fail due to that limitation, but it seems that works, so maybe was configured to not have a cap before?
|
@gastonponti Good point, that is strange there is a limit (see below), and the default which is used in the test is 64, so I'd really expect this test to at least be flaky if not fail all the time, but even running with a block time of 20 seconds, which means all txs will have been sent and in the tx pool before a block is constructed (300 txs per account) it still passes! So I guess there is some understanding we are missing, anyway I've updated the tx sending to try and not overload the account queue. celo-blockchain/core/tx_pool.go Line 193 in ff274ca |
Increased time between transactions sending so that accounts don't send more than 64 txs within the block period.
Adds a test to verify that the tx pool is executing concurrent fee currency checks safely.
I created this to see if I could reproduce #2318 but I was unable to reproduce that issue. It did however uncover a different race condition, which was that
core.currentwas being set in one goroutine and accessed from others without synchronisation. This was easy to fix by just locking around the point where it is set.